-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add kv rollback
#4774
Add kv rollback
#4774
Conversation
Like `kv patch` this is more of a helper than anything else; it provides a single command to fetch the current version (for CAS), read the version you want to roll back to, and set it as the new version (using CAS for safety).
|
||
// Now run it again and read the version we want to roll back to | ||
var data map[string]interface{} | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never seen this syntax before, do it create a new lexical scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Here it's just for logical separation so I could use the same mechanism but not worry about declaration vs assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats a nice trick, the lazy version of a func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's enough difference that a function having to check parameters would have ended up as long, and quite possibly more complicated.
command/kv_rollback.go
Outdated
return 2 | ||
} | ||
|
||
// Verify current data found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this step is necessary. If the version has been marked deleted there wouldn't be any data returned, but we should still allow a rollback on that key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
command/kv_rollback.go
Outdated
} | ||
|
||
if meta["deletion_time"] != nil && meta["deletion_time"].(string) != "" { | ||
c.UI.Error(fmt.Sprintf("Cannot roll back to a version that has been deleted")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you able to test if this check works? I think if the version has been deleted then no data would be returned and the above data checks would fail first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought if it was deleted it 404s but still returns metadata
. I'll check.
} | ||
|
||
if meta["destroyed"] != nil && meta["destroyed"].(bool) { | ||
c.UI.Error(fmt.Sprintf("Cannot roll back to a version that has been destroyed")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Like
kv patch
this is more of a helper than anything else; it providesa single command to fetch the current version (for CAS), read the
version you want to roll back to, and set it as the new version (using
CAS for safety).